-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Replace powerTip with bootstrap tooltips #972
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhawthorn
force-pushed
the
bootstrap_tooltips
branch
from
March 7, 2016 23:38
449553c
to
2ef6bf3
Compare
#955 has landed. Now this can be rebased @jhawthorn |
jhawthorn
force-pushed
the
bootstrap_tooltips
branch
2 times, most recently
from
March 8, 2016 23:31
f775068
to
0de2a0c
Compare
Phantomjs 1.9.8 (still our recommended version) doesn't support Function.prototype.bind. This adds a polyfill for it. If we ever choose to only support phantomjs >= 2.0, this can be removed, but we must first switch our CI and it would be polite to give our users a great deal of notice, as this will affect any tests they have of the backend.
jhawthorn
force-pushed
the
bootstrap_tooltips
branch
from
March 9, 2016 00:09
0de2a0c
to
162ec03
Compare
Out tooltips often are covering other interactive elements on the page, this allows interacting with those elements "through" the tooltip. Without this hovering over our rows of edit buttons feels clunky.
jhawthorn
force-pushed
the
bootstrap_tooltips
branch
from
March 9, 2016 00:44
162ec03
to
289c395
Compare
@tvdeyen done! |
Looks good to me, thanks @jhawthorn |
If we change the selector to something more specific and closer to the event target element, we are good to go! Nice work, John, as always 👌🏻 |
This won't work for tables dynamically added to the page, but at the moment we don't have any of those. From the jquery docs: Attaching many delegated event handlers near the top of the document tree can degrade performance. Each time the event occurs, jQuery must compare all selectors of all attached events of that type to every element in the path from the event target up to the top of the document. For best performance, attach delegated events at a document location as close as possible to the target elements. Avoid excessive use of document or document.body for delegated events on large documents.
jhawthorn
added a commit
that referenced
this pull request
Mar 9, 2016
Replace powerTip with bootstrap tooltips
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on #955
This swaps out the existing jQuery PowerTip tooltip library with bootstrap's tooltips.
One advantage of this is that bootstrap's tooltips can listen on the body for hover events on elements, which avoid the need to re-initialize tooltips when content is dynamically added to the page.